-
-
Notifications
You must be signed in to change notification settings - Fork 442
Save settings before shutdown/restart to prevent data loss #4025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Added a call to `Context.API.SaveAppAllSettings()` before executing system shutdown, restart, or advanced restart operations. This ensures that any unsaved settings are persisted, reducing the risk of data loss during these actions.
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds data loss prevention to system operations by saving application settings before executing shutdown, restart, and advanced restart commands. The change ensures that any unsaved application settings are persisted before the system powers down or reboots.
Key changes:
- Added
Context.API.SaveAppAllSettings()
calls before system operations - Applied the fix to all three power operations: shutdown, restart, and advanced restart
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Warning Rate limit exceeded@Jack251970 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds calls to save all application settings after user confirmation and before executing system actions (shutdown, restart, restart with advanced boot) in the system plugin’s main command handler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant P as Sys Plugin (Main.cs)
participant OS as Windows API
U->>P: Trigger shutdown/restart/advanced boot
P->>U: Show confirmation dialog
U-->>P: Confirm
Note right of P: New step added
P->>P: SaveAppAllSettings()
P->>P: Check privileges/elevation
alt Shutdown/Restart
P->>OS: ExitWindowsEx(...)
OS-->>P: Action initiated
else Advanced boot
P->>OS: Shutdown with advanced options
OS-->>P: Action initiated
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Sys/Main.cs (2)
286-287
: Consider saving settings before log off for consistency.Unlike shutdown/restart, this command doesn't save settings before executing. For consistency with the other system state-changing commands, consider adding
SaveAppAllSettings()
after the user confirms.if (result == MessageBoxResult.Yes) + { + // Save settings before log off to avoid data loss + Context.API.SaveAppAllSettings(); PInvoke.ExitWindowsEx(EXIT_WINDOWS_FLAGS.EWX_LOGOFF, REASON); + }
377-379
: Consider saving settings before exiting the application.The "Exit" command closes the application without saving settings, which could lead to data loss if the user has made configuration changes. Consider adding
SaveAppAllSettings()
before closing.Action = c => { + Context.API.SaveAppAllSettings(); Context.API.HideMainWindow(); Application.Current.MainWindow.Close(); return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Sys/Main.cs
(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Sys/Main.cs (2)
Flow.Launcher/PublicAPIInstance.cs (1)
SaveAppAllSettings
(108-117)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
SaveAppAllSettings
(60-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Plugins/Flow.Launcher.Plugin.Sys/Main.cs (1)
402-403
: RestartApp() already saves settings before restart
RestartApp() in PublicAPIInstance.cs calls SaveAppAllSettings() before invoking UpdateManager.RestartApp(), so no explicit save is required.
Previously, `Context.API.SaveAppAllSettings()` was called unconditionally before user confirmation for shutdown, restart, and advanced restart actions. This change ensures settings are only saved if the user confirms the action by clicking "Yes" in the confirmation dialog. For all three functionalities: - Moved the settings save call inside the `if (result == MessageBoxResult.Yes)` block. - Retained the existing logic for executing the respective system commands, with checks for `EnableShutdownPrivilege()` to determine whether to use `PInvoke.ExitWindowsEx` or the `shutdown` command. This change prevents unnecessary settings saves when the user cancels the action.
Removed logoff operation logic and associated return statement. Eliminated return statement after recycle bin error handling. Removed async plugin data reload and success message logic. Simplified theme selector query handling by removing `return false`. These changes streamline the code and improve maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Added a call to
Context.API.SaveAppAllSettings()
before executing system shutdown, restart, or advanced restart operations. This ensures that any unsaved settings are persisted, reducing the risk of data loss during these actions.